Speeding up adding factors + computing wiring #129
Conversation
antoine-dedieu
changed the title
antoine-dedieu
changed the title
antoine-dedieu
changed the title
Codecov Report
@@ Coverage Diff @@
## master #129 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 13 +3
Lines 854 912 +58
=========================================
+ Hits 854 912 +58
Continue to review full report at Codecov.
|
antoine-dedieu
force-pushed
the
speedup_factor_graph
branch
from
754272c to
4b389e0
Compare
There was a problem hiding this comment.
Made some detailed comments, although nothing major.
It looks like there should be a way to use numba parallel and significantly speed up wiring compilation while unifying the compile wiring interface for all factor groups. So although the current factor group wiring compilation is still a bit annoying, we can go with what we have now after some additional cleanups (since we are likely going to rewrite many of these in the next PR).
pgmax/fg/graph.py
Outdated
| def add_factor( | ||
| self, | ||
| variable_names: List, | ||
| factor_configs: np.ndarray, | ||
| log_potentials: Optional[np.ndarray] = None, | ||
| log_potentials: Optional[np.ndarray] = np.empty((0,)), |
There was a problem hiding this comment.
Why change None to empty?
There was a problem hiding this comment.
As I mentioned, for more consistency with the default FactorGroup parent
|
Thanks for your thorough review @StannisZhou |
StannisZhou
force-pushed
the
speedup_factor_graph
branch
from
b0c05ef to
e604c49
Compare
StannisZhou
force-pushed
the
speedup_factor_graph
branch
from
0998a56 to
46f2ab0
Compare
tests/fg/test_graph.py
Outdated
| @@ -32,7 +35,7 @@ def test_factor_graph(): | |||
| with pytest.raises( | |||
| ValueError, | |||
| match=re.escape( | |||
| f"A factor involving variables {frozenset([0])} already exists." | |||
| f"A {enumeration_factor.EnumerationFactor} involving variables {tuple([0])} already exists." | |||
There was a problem hiding this comment.
I updated fg.graph for this part. Please update this test accordingly!
The fix right now is not entirely correct for LogicalFactors, but a proper fix seems unnecessarily complicated. I will create a issue to keep track of this but for now we can go with the not entirely correct solution using frozenset
Main goal of this PR is to speed up the process of adding factors to a FactorGraph and to compile wiring. In particular
pgmax/groups/to clarify the structureBefore:
After:
Our next step will accelerate the wiring compilation further by using
numba